Warn if implicit default shadows given #23559

som-snytt · 2025-07-17T23:16:31Z

When supplying a default arg for an implicit application, warn if there is an implicit value available.

That is for f(using x = v) where a default is supplied in f(using x = v, y = default). The user might think the implicit value is supplied, which is not the case for explicit using; this is especially confusing in a recursive call, where the user intends to "replace" an implicit parameter.

Separately, behind a flag, warn if any recursive call uses a default arg instead of passing the current parameter value.

-Wrecurse-with-default, "Warn when a method calls itself with a default argument."

som-snytt · 2025-07-17T23:38:47Z

I didn't think to bootstrap; there are many instances, including the classic jsig!

[warn] 250 |            jsig(defn.ObjectType)
[warn]     |            ^^^^^^^^^^^^^^^^^^^^^
[warn]     |Argument for parameter toplevel was supplied using a default argument.

som-snytt · 2025-07-18T00:24:53Z

Could offer a quickfix that inlines the default.

odersky · 2025-07-18T10:31:12Z

I think this warning would be too noisy. Implicit defaults are there for a reason, we do not want to warn each time we use one.

Also the change set looks too big and risky for such a relatively minor improvment.

bracevac · 2025-07-18T12:06:55Z

It would be interesting to see how frequently this warning pops up in the open community build to have an understanding of how frequently these situations occur.

som-snytt · 2025-07-18T13:24:51Z

The recursion case is the Scala 2 lint. I find it very confusing for the reason cited on the ticket: was this intentional or an error? The compiler can't help you decide, but you feel better about it if someone had to type in p = true. The expectation is that such "config" is passed down the stack.

For the using case, it tries to warn only if a default was used when an implicit would have been supplied from scope; not sure I did that the best way, but how often is that actually intended?

In fact, it might be cleaner to refuse to supply defaults in those cases. However, I haven't spent time thinking abstractly about it.

Noisy warnings are usually behind a flag; I wasn't sure if the compiler team wants to go that route, since flags (even for warnings) are also a maintenance burden and a challenge for cross-builds (especially under -Werror).

som-snytt · 2025-07-18T13:44:23Z

This was the highlight of the effort.

Probably that is an ad for the mechanism described in the ticket to explicitly supply remaining implicits.

som-snytt · 2025-07-18T13:58:36Z

Not sure about indirect recursion (where Scala 2 warns for the same reason)

class C {
  def f(x: Int, s: String = ""): List[Int] = {
    List(new C).flatMap(_.f(42))
  }
}

For fun, I'll try doing the check at tailrec or tailcalls whatever.

som-snytt · 2025-07-18T14:54:42Z

Similar to the Scala 2 lint for "named boolean parameters" where you must name them if necessary to avoid ambiguity or accidentally switching args, one boolean parameter with a default might by OK (where there is one use site which requires the alternate behavior or state) but more than one is hard to follow, especially when recursing.

But that sounds like a third-party lint. (The named boolean lint was added as a project-internal lint on Scala 2.)

Implicit primitives add extra eye blinking.

som-snytt · 2025-07-18T16:22:42Z

Put the recursion under a flag, but leave the titular warning.

I didn't retry the scaladoc test, which probably requires the "special syntax" for a satisfying fix.

som-snytt · 2025-07-18T16:32:09Z

As suspected, the scaladoc case is noticed, appropriately.

[E219] Type Warning: /__w/scala3/scala3/scaladoc/src/dotty/tools/scaladoc/tasty/TypesSupport.scala:272:66

I will nowarn it for now, rather than munging the code.

odersky · 2025-07-18T17:17:39Z

The recursion case is the Scala 2 lint. I find it very confusing for the reason cited on the ticket: was this intentional or an error? The compiler can't help you decide, but you feel better about it if someone had to type in p = true. The expectation is that such "config" is passed down the stack.

I overlooked that. Yes, this is indeed confusing and should be avoided.

odersky

It's on the right track, but i would try to do without the UsedDefault attachment.

odersky · 2025-07-23T14:01:51Z

compiler/src/dotty/tools/dotc/reporting/messages.scala

+  override protected def explain(using Context): String =
+    "Usually it's intended to prefer the given in scope, but you must specify it explicitly."
+
+final class TailrecUsedDefault(using Context) extends TypeMsg(TailrecUsedDefaultID):


Since it's not restricted to tailrec calls, the class should be renamed, maybe to RecursiveUsesDefault?

odersky · 2025-07-23T14:02:18Z

compiler/src/dotty/tools/dotc/transform/TailRec.scala

+        if isRecursiveCall then
+          if ctx.settings.Whas.recurseWithDefault && tree.args.exists(_.hasAttachment(UsedDefaults)) then
+            report.warning(TailrecUsedDefault(), tree.srcPos)
+
          if (inTailPosition) {


It would be a tailrec call only if inTailPosition is true.

odersky · 2025-07-23T15:58:26Z

compiler/src/dotty/tools/dotc/typer/Applications.scala

@@ -42,6 +42,9 @@ import dotty.tools.dotc.inlines.Inlines
 object Applications {
  import tpd.*

+  /** Attachment indicating that an argument in an application was a default arg. */
+  val UsedDefaults = Property.StickyKey[Unit]


That will not work in from tasty compilation, since attachments are not pickled.

odersky · 2025-07-23T16:01:52Z

compiler/src/dotty/tools/dotc/transform/TailRec.scala

@@ -325,7 +326,10 @@ class TailRec extends MiniPhase {
          method.matches(calledMethod) &&
          enclosingClass.appliedRef.widen <:< prefix.tpe.widenDealias

-        if (isRecursiveCall)
+        if isRecursiveCall then
+          if ctx.settings.Whas.recurseWithDefault && tree.args.exists(_.hasAttachment(UsedDefaults)) then


Consider finding a more local way to figure this out, one that does not span most of the compiler with a time-travelling attachment. Maybe look at spans? Or else check whether the argument's symbol is a default getter.

odersky · 2025-07-23T16:08:30Z

compiler/src/dotty/tools/dotc/typer/Applications.scala

@@ -774,6 +777,14 @@ trait Applications extends Compatibility {
              methodType.isImplicitMethod && (applyKind == ApplyKind.Using || failEmptyArgs)

            if !defaultArg.isEmpty then
+              if methodType.isImplicitMethod && ctx.mode.is(Mode.ImplicitsEnabled)
+              && inferImplicitArg(formal, appPos.span).tpe.isError == false


Suggested change

&& inferImplicitArg(formal, appPos.span).tpe.isError == false

&& !inferImplicitArg(formal, appPos.span).tpe.isError

That was intentional, since the operator is so far away from the word it negates. I could introduce a var for such a case.

It also reads as "did not infer an implicit arg"...

-Wrecurse-with-default warns on self-recursion with default arg.

som-snytt · 2025-07-23T20:16:08Z

Reminder that this is especially brittle, even if it warns about unused nowarn. By the time my PR is merged, I don't know if the error IDs will have migrated. It needs a special comment to SYNC ERROR ID.

   // TODO #23 add support for all types signatures that make sense
+  @nowarn("id=E219")
   private def inner(

som-snytt · 2025-07-23T20:35:43Z

Only 13 warnings for compiler under -Wrecurse-with-default, which is narrower than warning when calling any enclosing method, but also feels like under-reporting to me. Choosing here to under-report ("false negative" or "less annoying").

The warnings for scaladoc:

[warn] -- [E219] Type Warning: /home/amarki/projects/scala3/scaladoc/src/dotty/tools/scaladoc/tasty/TypesSupport.scala:273:66
[warn] 273 |                val sig = inParens(inner(qual, skipThisTypePrefix)(using skipTypeSuffix = true), shouldWrapInParens(qual, tp, true))
[warn]     |                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[warn]     |Argument for implicit parameter indent was supplied using a default argument.
[warn]     |
[warn]     | longer explanation available when compiling with `-explain`
[warn] -- [E219] Type Warning: /home/amarki/projects/scala3/scaladoc/src/dotty/tools/scaladoc/tasty/TypesSupport.scala:280:43
[warn] 280 |            inner(qual, skipThisTypePrefix)(using skipTypeSuffix = true) ++ plain(".").l ++ suffix
[warn]     |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[warn]     |Argument for implicit parameter indent was supplied using a default argument.
[warn]     |
[warn]     | longer explanation available when compiling with `-explain`
[warn] -- [E219] Type Warning: /home/amarki/projects/scala3/scaladoc/src/dotty/tools/scaladoc/tasty/TypesSupport.scala:289:50
[warn] 289 |          case tp => inner(tp, skipThisTypePrefix)(using skipTypeSuffix = true) ++ plain(".").l
[warn]     |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[warn]     |Argument for implicit parameter indent was supplied using a default argument.
[warn]     |
[warn]     | longer explanation available when compiling with `-explain`
[warn] -- [E219] Type Warning: /home/amarki/projects/scala3/scaladoc/src/dotty/tools/scaladoc/tasty/TypesSupport.scala:308:132
[warn] 308 |            keyword(caseSpaces + "case ").l ++ inner(from, skipThisTypePrefix) ++ keyword(" => ").l ++ inner(to, skipThisTypePrefix)(using indent = indent + 2) ++ plain("\n").l
[warn]     |                                                                                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[warn]     |Argument for implicit parameter skipTypeSuffix was supplied using a default argument.
[warn]     |
[warn]     | longer explanation available when compiling with `-explain`
[warn] -- [E219] Type Warning: /home/amarki/projects/scala3/scaladoc/src/dotty/tools/scaladoc/tasty/TypesSupport.scala:310:132
[warn] 310 |            keyword(caseSpaces + "case ").l ++ inner(from, skipThisTypePrefix) ++ keyword(" => ").l ++ inner(to, skipThisTypePrefix)(using indent = indent + 2) ++ plain("\n").l
[warn]     |                                                                                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[warn]     |Argument for implicit parameter skipTypeSuffix was supplied using a default argument.
[warn]     |
[warn]     | longer explanation available when compiling with `-explain`
[warn] -- [E220] Type Warning: /home/amarki/projects/scala3/scaladoc/src/dotty/tools/scaladoc/tasty/TypesSupport.scala:166:47
[warn] 166 |            val parsedMethod = parseRefinedElem(name, t.resType)
[warn]     |                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[warn]     |                               Recursive call used a default argument.
[warn]     |
[warn]     | longer explanation available when compiling with `-explain`
[warn] -- [E220] Type Warning: /home/amarki/projects/scala3/scaladoc/src/dotty/tools/scaladoc/tasty/TypesSupport.scala:169:35
[warn] 169 |            } else parseRefinedElem(name, t.resType)
[warn]     |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[warn]     |                   Recursive call used a default argument.
[warn]     |
[warn]     | longer explanation available when compiling with `-explain`
[error] No warnings can be incurred under -Werror (or -Xfatal-warnings)
[warn] 7 warnings found

I will fix the E219s at least, where I think "use current implicit" was intended.

som-snytt · 2025-07-23T21:19:04Z

Much improved by following the review advice.

Rebased and squashed. The scaladoc fix is a separate commit in case my guesses were wrong.

I see it doesn't report the parameter name: Recursive call used a default argument.

som-snytt force-pushed the issue/23541-warn-tricky-defaults branch from 4bb349c to 9e53b78 Compare July 18, 2025 04:06

som-snytt force-pushed the issue/23541-warn-tricky-defaults branch from 9e53b78 to f038825 Compare July 18, 2025 16:20

som-snytt force-pushed the issue/23541-warn-tricky-defaults branch from f038825 to c7045ea Compare July 18, 2025 17:03

som-snytt marked this pull request as ready for review July 18, 2025 22:51

som-snytt force-pushed the issue/23541-warn-tricky-defaults branch from c7045ea to b144877 Compare July 20, 2025 02:07

Gedochao requested a review from odersky July 21, 2025 10:39

Gedochao assigned odersky Jul 21, 2025

odersky requested changes Jul 23, 2025

View reviewed changes

odersky assigned som-snytt and unassigned odersky Jul 23, 2025

Warn if implicit default shadows given

e82fe0a

-Wrecurse-with-default warns on self-recursion with default arg.

Use explicit args in recursion

27588b0

som-snytt force-pushed the issue/23541-warn-tricky-defaults branch from b144877 to 27588b0 Compare July 23, 2025 21:11

Name the parameter using a default

f9832df

som-snytt requested a review from odersky July 24, 2025 15:09

	&& inferImplicitArg(formal, appPos.span).tpe.isError == false
	&& !inferImplicitArg(formal, appPos.span).tpe.isError

Warn if implicit default shadows given #23559

Are you sure you want to change the base?

Warn if implicit default shadows given #23559

Uh oh!

Conversation

som-snytt commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

som-snytt commented Jul 17, 2025

Uh oh!

som-snytt commented Jul 18, 2025

Uh oh!

odersky commented Jul 18, 2025

Uh oh!

bracevac commented Jul 18, 2025

Uh oh!

som-snytt commented Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

som-snytt commented Jul 18, 2025

Uh oh!

som-snytt commented Jul 18, 2025

Uh oh!

som-snytt commented Jul 18, 2025

Uh oh!

som-snytt commented Jul 18, 2025

Uh oh!

som-snytt commented Jul 18, 2025

Uh oh!

odersky commented Jul 18, 2025

Uh oh!

odersky left a comment

Choose a reason for hiding this comment

Uh oh!

odersky Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

odersky Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

odersky Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

odersky Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

odersky Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

som-snytt Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

som-snytt Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

som-snytt commented Jul 23, 2025

Uh oh!

som-snytt commented Jul 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

som-snytt commented Jul 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

som-snytt commented Jul 17, 2025 •

edited

Loading

som-snytt commented Jul 18, 2025 •

edited

Loading

som-snytt commented Jul 23, 2025 •

edited

Loading

som-snytt commented Jul 23, 2025 •

edited

Loading